Skip to content

[ISSUE #10859] Fix exhausted tiered index write result - #11185

Open
beautyarbutin wants to merge 1 commit into
apache:developfrom
beautyarbutin:fix/tiered-index-write-result
Open

beautyarbutin wants to merge 1 commit into
apache:developfrom
beautyarbutin:fix/tiered-index-write-result

Conversation

@beautyarbutin

Copy link
Copy Markdown

Which Issue(s) This PR Fixes

Brief Description

IndexStoreService.putKey returned SUCCESS after all three index write attempts failed. This change retains the result of each attempt and returns the final failure once retries are exhausted, while preserving the existing success path and FILE_FULL file rotation behavior.

The regression test exercises FILE_FULL, FILE_FULL, and UNKNOWN_ERROR, then verifies that the final error is propagated after three attempts and that file rotation occurs only for the two FILE_FULL results.

How Did You Test This Change?

  • Confirmed the new regression test failed before the production change: expected UNKNOWN_ERROR but received SUCCESS.
  • Ran mvn -pl tieredstore -am -DskipITs -Dtest=IndexStoreServiceTest -Dsurefire.failIfNoSpecifiedTests=false test.
  • Result: 11 tests passed; Checkstyle reported 0 violations; SpotBugs reported no errors or warnings.

@RockteMQ-AI RockteMQ-AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Fixes a clear bug where IndexStoreService.putKey returned SUCCESS after all three write attempts failed. The fix correctly propagates the last failure result (AppendResult.UNKNOWN_ERROR or FILE_FULL) back to the caller.

Findings

  • [Critical] This was a data integrity bug — returning SUCCESS on failure could cause the caller to believe the index was written when it was not. Good catch.
  • [Info] The test putKeyReturnsLastFailureAfterRetriesTest is well-structured: uses Mockito to simulate sequential failures and verifies both the return value and the retry count (3 putKey calls, 2 createNewIndexFile calls).

Overall

Clean, minimal fix with proper test coverage. LGTM.


Automated review by RockteMQ-AI

@RockteMQ-AI RockteMQ-AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

New commits pushed since last review. The changes look good — addressing the previous feedback. LGTM.


Automated review by github-manager-bot

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] IndexStoreService reports success after index writes exhaust retries

2 participants